Removed dependencies in certain document-related classes#2073
Removed dependencies in certain document-related classes#2073Lehonti wants to merge 11 commits intoPintaProject:masterfrom
Conversation
|
There are likely going to be some merge conflicts from the DocumentWorkspace changes in #2046 so I'll wait to review this PR until that one lands |
|
@cameronwhite since the pull request you mentioned has been merged, I think we can merge this one. This is an intermediate step towards a specific goal. First, removing any dependencies on services from the constructor of |
|
Thanks, will review later this week! |
|
@cameronwhite thank you. And about the reported conflicts, I don't know why that might be. I am using the web UI and it doesn't report conflicts, which is why I left the changes as they are. Perhaps the pull request hasn't been refreshed locally? |
|
Ah, it had selected the "Rebase and merge" option - the squash mode seems fine so I think it's all good |
| public double Scale | ||
| => ViewSize.Width / (double) document.ImageSize.Width; | ||
|
|
||
| public void SetScale (double value, ToolManager tools) |
There was a problem hiding this comment.
I don't see this tools parameter being used anywhere in the method?
There was a problem hiding this comment.
You are right. I am not sure what the reason was for me to do this, and I can't test Pinta locally at this time, but after removing the parameter, the GitHub build actions succeed, so I think it's okay now
| public IEnumerable<BaseHistoryItem> Items => history; | ||
|
|
||
| public void PushNewItem (BaseHistoryItem newItem) | ||
| public void PushNewItem (BaseHistoryItem newItem, EditActions edit) |
There was a problem hiding this comment.
I think the future fix here would be to avoid the dependency entirely - if the EditActions instead listened for history events (similar to the history pad) then it could update the disabled state of the undo/redo actions without needing the Document to be aware of this
If that seems easy to get working, then maybe we just do that instead of introducing and then removing these parameters? Otherwise I'm okay with the changes as an intermediate step
There was a problem hiding this comment.
Yeah, that could very well be the case, and sorry if the refactoring seems a bit all over the place sometimes, but sometimes things that seem simple, like removing a dependency, end up touching a lot of files, and I have to 'slice' the planned changes to keep the difficulty of the review reasonable (like here...I planned to remove all dependencies from Document but I couldn't even get all of them out of DocumentWorkspace). In this case, Visual Studio reports that PushNewItem has 54 references. I would rather keep the refactoring you are suggesting for a future pull request, and frankly (at a first glance, looking at the call sites) I think it's quite a few intermediate steps away.
2 participants
Now the removed dependencies are passed as arguments to methods.
This comes at the cost of introducing references to
PintaCorein several places, which does not re-introduce them where they were before (which would trash our previous work): the dependencies are being obtained and coordinated at a more 'outer' layer of the code.Notice that some of the places where the
PintaCorereferences were introduced are tools (as inTextTool,PencilTool, ...). I can envision that being refactored so that the tools use services instead ofPintaCorereferences.The original goal was removing all dependencies (other than
Document) fromDocumentWorkspace, but it got quite large.